-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Response Ops][Task Manager] Add new configurable values #190934
Conversation
@@ -67,9 +73,17 @@ const requestTimeoutsConfig = schema.object({ | |||
|
|||
export const configSchema = schema.object( | |||
{ | |||
active_nodes_lookback: schema.number({ | |||
defaultValue: DEFAULT_ACTIVE_NODES_LOOK_BACK_S, | |||
min: 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we should set a max as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would 5 minutes make sense? If a user sets 5m, it would take 5m for a node that crashed to disappear from the list of active nodes. Perhaps its a sufficient high bound that we'd want to support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 730d933
allow_reading_invalid_state: schema.boolean({ defaultValue: true }), | ||
/* The number of normal cost tasks that this Kibana instance will run simultaneously */ | ||
capacity: schema.maybe(schema.number({ min: MIN_CAPACITY, max: MAX_CAPACITY })), | ||
discovery_interval: schema.number({ | ||
defaultValue: DEFAULT_DISCOVERY_INTERVAL_MS, | ||
min: DEFAULT_DISCOVERY_INTERVAL_MS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we should set a max as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see 1 or 5 minutes as the max, we could allow a smaller min
, maybe 1s
or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 730d933 to set max of 5 minutes
Pinging @elastic/response-ops (Team:ResponseOps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would putting these new discovery configs under a name space (like discovery.
) be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 730d933
@@ -32,6 +32,12 @@ export const DEFAULT_WORKER_UTILIZATION_RUNNING_AVERAGE_WINDOW = 5; | |||
export const CLAIM_STRATEGY_UPDATE_BY_QUERY = 'update_by_query'; | |||
export const CLAIM_STRATEGY_MGET = 'mget'; | |||
|
|||
export const DEFAULT_DISCOVERY_INTERVAL_MS = 1000 * 10; | |||
|
|||
export const DEFAULT_ACTIVE_NODES_LOOK_BACK_S = 30; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe duration format like 1s
1h
. we already have validateDurationSchema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 730d933
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Left two notes for nice-to-have. Not a blocker.
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @ymao1 |
Resolves #190734
Summary
Making the following configurable: